chore(lessons): promote both pending lessons, one as a gate and one as a rule - #597
Conversation
…s a rule Clears `.claude/pending-lessons.md` before Phase 7. Lesson 1 (checkable) becomes a gate in the merge sweeps. `gh pr update-branch` rewrites the head SHA and re-triggers the `review` check, but `reviewDecision` is PR-level and keeps the pre-update APPROVED while that re-review runs, so both sweeps could squash-merge on a snapshot that predated the head they were merging. That is how a HIGH backend-contract finding reached main on orbit-api#403 and the fix landed on the orphaned head branch instead. Both scripts now: - discover whether the repo runs `.github/workflows/claude-review.yml`, and if it does, refuse every merge path until the `review` check on the CURRENT head SHA is terminal, then re-read reviewDecision; - record each merged PR's head branch and re-check it at end of sweep, printing ORPHANED-HEAD and exiting 1 when a post-merge push re-created it; - gain `--help`, documented exit codes (0/1/2), and a timeout line that names the actual blocker instead of a generic "waiting for CLEAN". `merge-sweep.sh` carried the same race in a stronger form (it never waited for checks at all), so it is fixed in the same pass rather than left as known-broken. Lesson 2 (judgment) becomes standing guidance in the orchestrate skill's delegation-discipline section, which is the one place in the repo that carries a delegation contract template and a wait-on-CI loop. Both halves land: subagents poll in the foreground inside their own turn, and the parent treats a "standing by / monitor armed" completion as a nudge trigger, not as progress.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #597
Scope: PR #597 in thomasluizon/orbit-ui-mobile (chore/promote-pending-lessons → main)
Recommendation: NEEDS WORK
Summary
The PR graduates two pending lessons: tools/merge-sweep.sh and tools/merge-sweep-cov.sh gain a
review-staleness guard (blocking merge until the review check on the current head SHA settles,
per the orbit-api #403 incident) and an end-of-sweep orphaned-head-branch scan; --help/exit-code
support is added per tools/CONVENTIONS.md; the second lesson lands as prose in
.claude/skills/orchestrate/SKILL.md. .claude/pending-lessons.md and tools/README.md are
updated in step, and the dash baseline shrinks correctly (verified: zero em dashes remain in either
script). The mechanism is sound in the case the PR's own test transcript exercises, but the
review_required gate that turns the whole safety mechanism on is itself computed by an
unpaginated, error-swallowed gh api call that silently reverts to the pre-patch (unsafe) behavior
on failure — a correctness gap in the exact code path this PR exists to harden.
Findings
Critical
None.
High
[HIGH] review-staleness guard silently disables itself on an unpaginated/failed workflow lookup
· dimension: 1. Correctness (this is the PR's own stated guarantee failing silently)
· location: orbit-ui-mobile/tools/merge-sweep.sh:55 (identical logic at orbit-ui-mobile/tools/merge-sweep-cov.sh:62)
· issue: `review_required` — the single switch that turns the new review-staleness guard on — is
set from `gh api "repos/$repo/actions/workflows" --jq '.workflows[].path' 2>/dev/null | grep -qx
"$REVIEW_WORKFLOW_PATH"`. `gh api` does not auto-paginate (opt-in via `--paginate`), and GitHub's
"List repository workflows" endpoint defaults to `per_page=30`; a repo with more than 30 workflow
files can return `claude-review.yml` on page 2+, which this call never fetches. Separately,
`2>/dev/null` swallows ANY failure of the call (auth, rate limit, transient network) with no
fallback. In every one of these cases `review_required` stays empty (not an error, just silently
unset) — verified end-to-end: it is read exactly once, at merge-sweep.sh:96 / merge-sweep-cov.sh:129,
and when empty, `review_stale` is never set, so the loop merges as soon as `ms` is CLEAN/UNSTABLE
and `rev=APPROVED` with zero wait for the current SHA's `review` check. That is exactly the
pre-patch behavior that caused orbit-api #403 (a HIGH backend-contract finding merged and deployed
before the re-review landed) — the one incident this PR exists to prevent.
· risk: Currently latent for orbit-ui-mobile (20 workflow files today, confirmed via `ls
.github/workflows`, under the 30-item page default), but silent and undetectable when it does
trip — no error line, no warning, just a merge that shouldn't have happened. It also reintroduces
the original bug on ANY transient `gh api` failure today, regardless of workflow count, since
fail-open (guard skipped) is the wrong default direction for a safety gate — it should fail closed
(assume the guard is required) when the check itself can't be confirmed.
· fix: Add `--paginate` to the `gh api "repos/$repo/actions/workflows"` call (or `-F per_page=100`,
since workflow counts realistically stay under 100) so the lookup can't silently truncate. Also
invert the failure direction: default `review_required=1` and only clear it when the `gh api` call
positively succeeds AND positively confirms the workflow file is absent, rather than defaulting to
"not required" and only setting it on a confirmed hit. That makes an API hiccup fail toward extra
waiting (safe, at worst a slower sweep) instead of toward skipping the guard (unsafe).
· reference: CLAUDE.md rule 8 (error handling at boundaries; a trust-boundary call like this needs a
defined failure behavior, not a silent swallow); rubric dimension 1 (Correctness — the diff's own
stated guarantee).
Medium
[MEDIUM] orphaned-head-branch check can false-positive on a delayed or failed branch delete
· dimension: 1. Correctness / 3. SOLID (missing edge case in new logic)
· location: orbit-ui-mobile/tools/merge-sweep.sh:119-128; orbit-ui-mobile/tools/merge-sweep-cov.sh:160-169
· issue: The end-of-sweep scan treats "branch exists" as proof the branch was "re-created after the
merge, so its commits are NOT on main." It never compares the branch's current tip SHA to the SHA
that was actually merged — it only checks existence via `gh api repos/$repo/branches/$branch`. A
branch can still exist after a successful `--squash --delete-branch` merge for reasons that have
nothing to do with a post-merge push: `--delete-branch`'s own delete step failing separately from
the merge (branch protection on the head branch, an insufficient token scope, a transient error)
and the deletion itself lagging behind the read. That race is sharpest for the single-PR
invocation shown in the PR's own test transcript (`merge-sweep-cov.sh <repo> 403`), where the
orphan check runs immediately after that PR's own merge with no other PRs' processing time to
absorb the delay.
· risk: A benign "branch didn't get deleted" is reported as "ORPHANED-HEAD ... commits are NOT on
main" (a factually wrong claim — the squash-merge to main already happened) and the script exits 1,
which any caller (the nightly sweep, a human running it ad hoc) will read as "an incident
happened," triggering unnecessary investigation or fix-forward action.
· fix: Record the merged PR's `headRefOid` (already available from `gate()`'s SHA field in
merge-sweep-cov.sh; add it to `mstate()` in merge-sweep.sh) alongside the branch name, and at
scan time compare the still-existing branch's current tip SHA to that recorded SHA. Only flag
ORPHANED-HEAD when the tip SHA differs from (or is ahead of) the merged SHA — proving a new commit
landed on the branch post-merge — not merely when the branch still exists.
· reference: rubric dimension 1 (Correctness — boundary condition: delete-branch failure vs.
genuine recreation); tools/CONVENTIONS.md "Gate tools" (a verdict should be computed from hard
evidence, not a proxy that a benign condition can also satisfy).
Low / Info
None posted (signal gate: style-only observations excluded).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or packages/shared/src/i18n/* changed |
| contract-aligner | N/A — no orbit-api or packages/shared/src/types/*/endpoints.ts change |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/*/orbit-landing-page UI file changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A — this review session's sandbox blocks executing repo scripts/shell commands beyond git/gh; not run |
| Type check | N/A — same sandbox restriction |
| Tests | N/A — same sandbox restriction |
| Build (api) | N/A — diff does not touch orbit-api |
Deferred — N/A dimensions & files not verdicted
- DESIGN.md / AI-slop (#8): N/A — no
apps/*UI files in the diff. - Parity (#9): N/A — no
apps/web/**orapps/mobile/**files in the diff. - i18n (#10): N/A — no i18n JSON or user-facing string changes.
- Contract drift + backward-compat (#11): N/A — no
packages/shared/src/types/*,endpoints.ts,
ororbit-apiDTO changes. - Security (#12, backend categories): N/A — no
orbit-apicode changed. Frontend-security
categories (XSS, auth-state leakage) also N/A — no frontend code in this diff. - Backend hard rules (#13): N/A — no
orbit-apichanged. - FEATURES.md parity (#14): N/A — no user-facing feature surface change (internal tooling only).
- Validation (Phase 7): not executed — this review session's tool sandbox declined to run shell
scripts/node/npmcommands (repeated "requires approval" on every attempt beyondgit/gh
read commands); the PR's own verification block (node tools/check-frontmatter.mjs,
node .claude/hooks/test-hooks.mjs,node tools/check-dashes.mjs --check-baseline,bash -non
both scripts) was not independently re-run. Manually verified instead: grepped both scripts for
the em-dash character and confirmed zero remain, matching thedash-baseline.jsondeletions;
manually traced both scripts' bash control flow line-by-line for syntax and logic soundness in
lieu ofbash -n. - All 6 changed files received a verdict; nothing was skipped for size or scope.
What's good
- The core review-staleness mechanism (poll the CURRENT head SHA's
reviewcheck to a terminal
state before trustingreviewDecision, sincereviewDecisionis PR-level and survives an
update-branch) correctly targets the actual root cause of orbit-api #403, and the fix is applied
to both sibling scripts (merge-sweep.shandmerge-sweep-cov.sh) rather than leaving one as a
known-broken twin, matching the PR body's own reasoning. --help, documented exit codes (0/1/2), and theusage()-on-bad-args path bring both
scripts up totools/CONVENTIONS.md's contract, which they previously lacked.- The orphaned-head-branch scan (existence check aside — see Medium finding) is a reasonable
secondary signal for detecting a past occurrence of the original bug class. .claude/pending-lessons.mdcorrectly clears the queue and records where each lesson landed with
dates, andtools/README.mdcatalog rows were updated in the same PR to describe the new
behavior — no stale documentation left behind.dash-baseline.jsonshrinks by exactly the count of em dashes actually removed (verified by grep
against both files: zero remain).
Recommendation
Fix the High finding before merge: make the review_required detection resilient to pagination and
API failure, and fail closed (assume the guard is needed) rather than fail open when the workflow
lookup can't be confirmed — this is the exact silent-failure shape the PR itself exists to close.
The Medium finding (SHA-based orphan detection instead of bare existence) is worth fixing in the
same pass since it touches the same block of new code, but would not block merge on its own.
…ot existence Addresses both review findings on #597. HIGH: `review_required`, the single switch enabling the review-staleness guard, was computed from an unpaginated, error-swallowed `gh api ... /actions/workflows` call and defaulted to OFF. `gh api` does not auto-paginate (the endpoint defaults to per_page=30), and `2>/dev/null` hid auth, rate-limit and network failures, so either condition silently restored the exact pre-patch merge behaviour the guard exists to prevent. Now `--paginate`, and the default inverts: start required, and clear it only when the lookup SUCCEEDS and positively shows no claude-review.yml. A failed lookup warns on stderr and keeps waiting. This is the same rule as the locked "Fail-closed the completion gate: a verifier error is not a clean pass" decision, applied one level up, to the guard's own precondition check. MEDIUM: the orphaned-head scan treated "branch still exists" as proof of a post-merge push, but `--delete-branch` can fail or lag independently of the merge, which would report a benign leftover branch as an incident and exit 1. Both scripts now record the head SHA that was actually merged and flag ORPHANED-HEAD only when the surviving branch's tip has MOVED past it. `mstate()` in merge-sweep.sh gained the headRefOid field it needed for this.
There was a problem hiding this comment.
Code Review: PR #597
Scope: PR #597 in orbit-ui-mobile — chore(lessons): promote both pending lessons, one as a gate and one as a rule
Recommendation: APPROVE
Summary
This PR hardens tools/merge-sweep.sh and tools/merge-sweep-cov.sh against the review-staleness race that let a HIGH backend-contract finding reach main on orbit-api#403 (reviewDecision is PR-level and survives an update-branch push, so a sweep could merge on a stale pre-update APPROVED while the re-triggered review check was still running on the new head SHA). Both scripts now block every merge path — including the coverage-only admin override — until the review check on the current head SHA settles, fail closed on an unreadable workflow list (paginated gh api ... --paginate, default-required unless the lookup positively proves the workflow absent), and scan merged PRs' surviving head branches for a tip that moved past the merged SHA (not mere existence, which would false-positive on a slow --delete-branch). It also gains --help, documented exit codes, and graduates two pending-lessons.md entries. Diff is scoped entirely to .claude/ and tools/; no apps/*, packages/shared, or orbit-api files are touched, so the platform-parity/i18n/contract/design/backend-security dimensions are all N/A by gate.
I read both scripts end to end and traced the guard's control flow against the race it targets: the review_required fail-closed default, the per-iteration re-fetch of reviewDecision alongside the review check's settle state (so a flip to CHANGES_REQUESTED is picked up in the same gh pr view payload that observes the check settling), the coverage-admin path sitting after the staleness gate in merge-sweep-cov.sh, and the SHA-based (not existence-based) orphan check. All of it holds up, including the two edge cases the PR's own proof output claims (case 4b: failed workflow lookup keeps the guard on; case 5b: a branch that merely survived --delete-branch with an unmoved tip is not reported). The two findings this PR's own body lists as already fixed (unpaginated/error-swallowed workflow lookup defaulting open; existence- vs SHA-based orphan check) are present and correctly implemented in the diff as pushed — nothing further to flag there.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] merge-sweep.sh has no fast-path SKIP when reviewDecision is not APPROVED, unlike its sibling
· dimension: SOLID / clean architecture (rule 10, cross-script consistency) — pattern inconsistency
· location: tools/merge-sweep.sh:82-122
· issue: merge-sweep-cov.sh's loop (tools/merge-sweep-cov.sh:112-116) checks `[ "$rev" != "APPROVED" ]`
first and SKIPs immediately with an explicit `review=$rev` message. merge-sweep.sh has no equivalent
early exit: once the review check settles to CHANGES_REQUESTED, `review_stale` clears but the merge
condition at line 107 still requires `rev = APPROVED`, so a non-approved PR just falls through to
`sleep 20` and loops for up to the full ~50 iterations before printing a generic
"SKIP #$n (timeout: never reached a mergeable state (ms=... rev=CHANGES_REQUESTED))".
· risk: not a correctness bug (it still correctly refuses to merge), but every rejected/changes-requested
PR in a batch costs the sweep up to ~17 minutes of pointless polling instead of exiting immediately,
and the eventual message reads as a generic timeout rather than the clear, fast `SKIP #$n review=$rev`
its sibling script gives. This is exactly the kind of drift between the two scripts that
`tools/README.md`'s "like merge-sweep.sh, but…" framing implies shouldn't exist.
· fix: add the same early check used in merge-sweep-cov.sh right after the `failed`/`DIRTY` checks:
`if [ "$rev" != "APPROVED" ]; then echo "SKIP #$n review=$rev"; done_pr=1; break; fi`
· reference: CLAUDE.md rule 10 (DRY/consistency at the right level); tools/CONVENTIONS.md ("one clear
purpose per script" — the two sweep scripts are meant to behave identically apart from the
Sonar-coverage carve-out)
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or i18n JSON changed |
| contract-aligner | N/A — no packages/shared/src/types/* / endpoints.ts, and only one repo changed |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/web, apps/mobile, or orbit-landing-page/src UI file changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A — CI wrapper skips /validate; Build/Unit Tests/SonarCloud run as separate required checks |
| Type check | N/A — same |
| Tests | N/A — same |
| Build (api) | N/A — orbit-api not touched |
Manually verified: bash -n on both scripts (syntax), field-order alignment between each mstate/gate
JS emitter and its corresponding bash read, and that the --paginate/fail-closed workflow lookup and
SHA-based (not existence-based) orphan check the PR body claims are actually present in the diff as
pushed.
Deferred — N/A dimensions & files not verdicted
- Parity / i18n / contract-drift / backend-security / backend-hard-rules / FEATURES.md parity: all N/A,
gated out because the diff touches only.claude/andtools/(noapps/*,packages/shared,
orbit-api, or user-facing feature surface). - Comment-policy dimension (#4): scoped by the rubric to
no-comments.cjs(TS/JS) andORBIT0001(C#);
bash isn't gated by either, so the new inline race-condition comments in the two scripts were read for
quality but not held to the JSDoc/WHY-URL lint bar. .claude/pending-lessons.md: the newly Graduated "2026-07-24 background subagents idle on phantom
background waiters" entry never appears as a## Pendingqueue row anywhere in this file's history —
only the "2026-07-14 sweep-merge" entry was actually staged there before this PR. This is a bookkeeping
observation on a prose changelog, not a code defect, so it is not raised as a Low finding per the
Signal gate; noted here for the author's awareness.- All six changed files (
.claude/pending-lessons.md,.claude/skills/orchestrate/SKILL.md,
tools/README.md,tools/dash-baseline.json,tools/merge-sweep-cov.sh,tools/merge-sweep.sh) were
read and verdicted; nothing left uncovered.
What's good
- The core race fix is correct and precisely targeted: the guard re-fetches
reviewDecisionand the
reviewcheck's settle state together in onegh pr viewcall every poll, so a flip to
CHANGES_REQUESTEDis caught in the same read that observes the check going terminal — no window for
a staleAPPROVEDto slip through, including on the coverage-admin override path.
(tools/merge-sweep-cov.sh:133-139,tools/merge-sweep.sh:98-106) - Fail-closed default on the workflow lookup (
review_required=1unless a successful lookup positively
shows noclaude-review.yml) correctly means an auth/rate-limit hiccup makes the sweep slower, never
less safe. (tools/merge-sweep.sh:55-62,tools/merge-sweep-cov.sh:62-69) - The orphan-head scan is SHA-based, not existence-based, so a branch that merely outlives
--delete-branch's eventual-consistency lag is correctly not reported — only a tip that actually moved
past the merged commit is. (tools/merge-sweep.sh:125-139,tools/merge-sweep-cov.sh:166-180) - New
--help/usage text, documented exit codes, and thetools/README.mdcatalog rows are accurate to
the new behavior and matchtools/CONVENTIONS.md's contract. - The PR body's own reproduction table (stub-
ghcases 1-7) is a genuinely good piece of evidence for a
concurrency fix that's otherwise hard to test.
Recommendation
Approve as-is. The one Medium finding (merge-sweep.sh's missing fast-path SKIP on a non-approved PR) is a
minor efficiency/consistency gap, not a safety issue — the script still correctly refuses to merge, it
just takes longer to say so. Fine to land now and pick up in a follow-up if the author wants the two
scripts to match exactly.
There was a problem hiding this comment.
Code Review: PR #597
Recommendation: APPROVE
Summary
Promotes both queued entries in .claude/pending-lessons.md: a checkable lesson that becomes a fail-closed merge guard in tools/merge-sweep.sh / tools/merge-sweep-cov.sh (block every merge path until the review check on the current head SHA settles, then re-read reviewDecision, plus an end-of-sweep scan for orphaned head branches), and a judgment lesson landed as prose in .claude/skills/orchestrate/SKILL.md. Pure tooling + docs diff, no apps/*, orbit-api, or packages/shared surface touched. The guard logic was traced step by step against every branch (APPROVED/CHANGES_REQUESTED, BEHIND, DIRTY, Sonar FAILURE, workflow-lookup failure, orphaned head) and is sound; one Medium defense-in-depth gap survived in the new orphan-scan error handling.
Findings
Critical
None.
High
None.
Medium
Orphan-scan swallows non-404 gh api failures as "no orphan", the same fail-open pattern this PR eliminates elsewhere
- location:
tools/merge-sweep.sh:134,tools/merge-sweep-cov.sh:175 - issue:
tip=$(gh api "repos/$repo/branches/$branch" --jq .commit.sha 2>/dev/null) || continuetreats every non-zerogh apiexit identically. A confirmed 404 (branch genuinely deleted by--delete-branch, the expected/benign case) and a transient failure (rate limit, network blip, auth hiccup) both silentlycontinuewith no diagnostic, so the run reports "clean" (exit 0, noORPHANED-HEADline) whether or not that PR's branch was actually verified. - risk: this is the identical fail-open shape the PR's own "Why the precondition check itself fails closed" section calls out and fixes for the workflow-lookup a few lines above in the same diff — the fix wasn't carried to this second lookup. If the orphan check silently no-ops during a transient error, a real orphaned head (the exact HIGH-severity failure mode from orbit-api#403 that this PR exists to catch) goes unreported and is never re-checked later, since
merged_headsis per-invocation, in-memory state. - fix: distinguish "confirmed deleted" from "could not verify" (e.g. check for a literal
404via--include), and only silently skip on a confirmed 404. On any other failure, print aWARN: could not verify branch <branch> for #<pr>; orphan status unknownto stderr, mirroring the existingWARN: could not list ... workflowspattern used one lookup above. - reference: CLAUDE.md rule 1 (root-cause consistency within the same diff); rubric dimension 3 (defense-in-depth gap, Medium).
Low / Info
None posted (signal gate).
Subagents
All gated N/A — the diff touches no apps/web/**, apps/mobile/**, orbit-api, packages/shared/src/types/*, or i18n JSON. Frontend-security categories reviewed inline instead: no injection surface (repo/pr only ever passed as gh argv, never eval'd); the two check/workflow-name interpolations into the inline node -e scripts are hardcoded constants, not user input.
Validation
Lint / type check / tests / build: N/A — diff is .sh + .md + .json only, no ESLint/Roslyn/tsc-covered surface changed, and this repo has no CI gate for shell syntax. Dash-baseline removal independently verified (git show <ref>:<file> | grep -c '—' = 0 for every changed prose file). Shell syntax verified by manual trace of every case/esac, if/fi, for/done, and function brace, rather than an automated bash -n run (blocked in this review session's sandbox).
Deferred
Dimensions 8/9/10/11/13/14 (DESIGN.md, parity, i18n, contract drift, backend hard rules, FEATURES.md) — N/A by gate, no matching surface in the diff. Phase 5 backward-compat guard — N/A, no shared-type/DTO hunks to classify. All 6 changed files received a verdict.
What's good
The review-staleness guard is genuinely correct end to end (traced through approved-settle, changes-requested-settle, workflow-absent-repo, and workflow-lookup-failure branches). The workflow-lookup fail-closed fix (--paginate, inverted default, WARN on failure) is exactly right. Both scripts gained --help and documented exit codes per tools/CONVENTIONS.md. The pr^branch^sha orphan-tracking encoding is small and correctly parsed.
Recommendation
Approve as-is. The one Medium finding is a real but narrow defense-in-depth gap in a post-merge, best-effort detection pass — worth a fast follow-up, not a blocker.
Addresses the Medium finding on the approving review of #597: the orphan scan's `gh api repos/<repo>/branches/<b> || continue` treated a confirmed 404 (the expected case, the branch really was deleted) and a transient failure (rate limit, network, auth) identically, so a run could report exit 0 with no ORPHANED-HEAD line without having verified anything. That is the same fail-open shape this PR removes from the workflow lookup twenty lines above. Both scripts now resolve the ref through GraphQL, which exits 0 with an EMPTY oid for a deleted branch. A non-zero exit is therefore unambiguously "could not verify": it warns on stderr and exits 3, a distinct code from 1 (a real orphaned head). Silence now means verified, per the locked "a verifier error is not a clean pass" decision.
There was a problem hiding this comment.
Code Review: PR #597
Recommendation: APPROVE
Summary
Tooling/process PR: promotes both .claude/pending-lessons.md entries (one as a gate, one as a rule), hardens tools/merge-sweep.sh and tools/merge-sweep-cov.sh against the orbit-api #403 race (update-branch re-triggers the review check while the PR-level reviewDecision still reports the stale pre-update value), adds an end-of-sweep orphaned-head-branch scan proven by SHA rather than existence, and lands "waiting is foreground work" guidance in .claude/skills/orchestrate/SKILL.md. No apps/*, packages/shared, or orbit-api files touched. I traced both scripts' control flow line-by-line against the PR's embedded 7-case stub-harness proof and independently re-derived the same behavior; the review-staleness guard correctly gates before the coverage-only admin-override path too, not just the plain-merge path — the one place a bypass could have slipped back in.
Findings
Critical: None. High: None. Medium: None. Low/Info: none posted per the rubric's signal gate.
Subagents (all N/A — diff touches only .claude/ and tools/)
parity-checker N/A · i18n-syncer N/A · contract-aligner N/A · security-reviewer N/A · design-reviewer N/A
Validation
No lintable/testable app surface changed. bash -n, check-frontmatter.mjs, and check-dashes.mjs --check-baseline were not re-executed in this sandboxed session (execution required an approval this environment could not grant); the PR body's own verification transcript reports all green, and I independently confirmed via gh pr diff that no added line introduces an em/en dash, matching the dash-baseline.json shrink.
Deferred
Dimensions 8/9/10/11/13/14 (DESIGN.md, parity, i18n, contract drift, backend hard rules, FEATURES.md) are N/A — the diff never touches their gated surface. Lint/type-check/test execution not independently re-run this session; PR's own transcript is the evidence of record. All 6 changed files received a verdict; nothing skipped for size.
What's good
- Review-staleness guard is ordered before the Sonar coverage-only
--adminoverride inmerge-sweep-cov.sh, so the one branch-protection-bypassing path isn't a back door around the new guard. - Fail-closed by construction: a failed/unpaginated workflow lookup now defaults to "review required" instead of silently disabling the guard (closes the hole the first draft shipped with, per the PR's own case 4b).
- Orphan scan proves drift by comparing the recorded merged SHA against the branch's current tip via GraphQL (empty oid = confirmed-deleted, non-zero exit = confirmed-unknown), not branch existence alone.
--help, documented exit codes (0/1/2/3), cwd-independent and non-interactive pertools/CONVENTIONS.md.- Watch-item, not a finding:
merge-sweep.sh/merge-sweep-cov.shnow duplicate a fair amount of new logic (workflow lookup,branch_tip(), orphan-scan loop) verbatim. Per CLAUDE.md rule 6 ("extract on the third real use"), two scripts don't clear the bar yet for a shared helper — worth revisiting if a third sweep script appears.
Recommendation
Merge as-is.
There was a problem hiding this comment.
Code Review: PR #597
Scope: PR #597 in thomasluizon/orbit-ui-mobile
Recommendation: APPROVE
Summary
This PR promotes both queued .claude/pending-lessons.md entries. The checkable lesson becomes a
fail-closed review-staleness + orphaned-head guard added to tools/merge-sweep.sh and
tools/merge-sweep-cov.sh; the judgment lesson becomes standing guidance in
.claude/skills/orchestrate/SKILL.md. The diff is entirely tooling/docs (no apps/*,
packages/shared, or orbit-api files), so the design, parity, i18n, contract-drift, security,
backend-hard-rules, and FEATURES.md dimensions are all out of surface. The two shell scripts
(safety-critical, since they auto-merge PRs to main) got the closest read; the added guard logic
is sound and does fix the documented #403 race, with one concrete edge-case gap below.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] Branch-name lookup for the orphan-scan can fail silently and skip verification without counting it as unverified
· dimension: 1. Correctness / 5. No-workaround (error handling, CLAUDE.md rule 8)
· location: orbit-ui-mobile/tools/merge-sweep.sh:109 and orbit-ui-mobile/tools/merge-sweep-cov.sh:97 (squash_merge)
· issue: Right before merging, both scripts fetch the branch name with a second, independent gh pr view … --json headRefName --jq .headRefName 2>/dev/null call. If that call fails (network blip, secondary rate limit) it silently yields an empty string; the merge still proceeds and merged_heads gets an entry with an empty branch. In the orphan-scan loop, [ -n "$branch" ] || continue then skips that entry entirely — it is not added to unverified and never printed as a WARN, so it neither shows up in the report nor trips exit code 3.
· risk: This is the exact failure mode both scripts' own header comments and exit-code contract explicitly promise never happens ("unknown is not a clean pass", exit 3 documented for "a head branch could not be verified"). A transient API hiccup at merge time silently drops that one PR out of the orphan check, and the sweep still reports SWEEP-DONE / exit 0 as if every merged head had been verified clean.
· fix: Don't make a second network call at all — add headRefName to the existing gate/mstate gh pr view --json ... call that already runs every poll iteration (it already succeeded, since that's the data driving the merge decision), and use that value directly in squash_merge/the merge branch. That removes the extra failure surface entirely. If a separate call is kept, at minimum treat an empty branch the same as a failed branch_tip lookup: warn on stderr and increment unverified instead of continue.
· reference: CLAUDE.md rule 8 (error handling — never swallow errors silently); tools/CONVENTIONS.md "Gate tools" (a gate's verdict must be computed from artifacts on disk, never silently assumed clean).
Low / Info
None.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or i18n JSON changed |
| contract-aligner | N/A — no packages/shared/src/types/*/endpoints.ts or orbit-api DTOs changed |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/* or orbit-landing-page UI files changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A — this review session's sandbox blocks node/npm execution; diff has no TS/JS/C# files to lint |
| Type check | N/A — same reason; no typed source changed |
| Tests | N/A — same reason; the PR's own proof transcript (stub-gh harness, scratchpad-only) is in the PR body |
| Build (api) | N/A — orbit-api not touched |
Deferred — N/A dimensions & files not verdicted
- 8. DESIGN.md / AI-slop — N/A, no
apps/*UI files touched. - 9. Parity — N/A, no
apps/web/**/apps/mobile/**files touched. - 10. i18n — N/A, no user-facing strings or locale JSON touched.
- 11. Contract drift + backward-compat — N/A, no
packages/shared/src/types/*or orbit-api DTOs touched. - 12. Security — N/A, no
orbit-api, noapps/*trust-boundary code touched; the shell scripts run only againstgh/GitHub's API with no new secret handling. - 13. Backend hard rules — N/A, orbit-api not touched.
- 14. FEATURES.md parity — N/A, purely internal tooling/process change, no user-facing feature surface.
- Phase 7 Validate — not executable in this review sandbox (Bash calls to
node/npmrequire an approval this session does not have); the PR body's own verification transcript (check-frontmatter.mjs,test-hooks.mjs,check-dashes.mjs --check-baseline,bash -non both scripts) is the available evidence and was not independently re-run here. - Every changed file (
tools/merge-sweep.sh,tools/merge-sweep-cov.sh,tools/README.md,
tools/dash-baseline.json,.claude/pending-lessons.md,.claude/skills/orchestrate/SKILL.md)
received a verdict above; nothing else deferred.
What's good
- The core fix is correct and well-targeted: gating every merge path on the
reviewcheck for the
current head SHA reaching a terminal state before re-readingreviewDecisiongenuinely closes
the #403 race (a stale PR-levelreviewDecisionsurviving a re-triggered check). - The workflow-lookup precondition is properly fail-closed now:
--paginatefixes the page-30 cap
bug, and a failed lookup defaultsreview_required=1(warns on stderr) rather than silently
disabling the guard — this is the right shape for a gate whose own precondition check can fail. - The orphaned-head scan's "moved past" comparison (vs. "branch merely survived deletion") correctly
avoids the false-positive the PR's own changelog says a prior draft had, and separates "confirmed
orphaned" (exit 1) from "could not verify" (exit 3) instead of collapsing unknown into clean. --help, explicit usage, and documented exit codes bring both scripts in line with
tools/CONVENTIONS.md.- The judgment-lesson landing in
orchestrate/SKILL.md's existing "Delegation discipline" section
(rather than the always-loaded.claude/rules/core.md) matches the stated rationale: it's read
only when delegating, not paid for on every turn.
Recommendation
Approve as-is. The one Medium finding is a narrow, well-scoped follow-up (fold headRefName into
the already-polled JSON instead of a second lookup) — worth fixing soon since it's cheap and removes
a real gap in the exact invariant this PR exists to establish, but it does not block merge.
There was a problem hiding this comment.
Code Review: PR #597
Scope: PR #597 in thomasluizon/orbit-ui-mobile (chore/promote-pending-lessons → main), head bdb63a7a
Recommendation: APPROVE
Summary
Pure tooling/docs diff (.claude/, tools/ only — no apps/*, packages/shared, or orbit-api). It graduates both queued .claude/pending-lessons.md entries: a fail-closed review-staleness + orphaned-head guard added to tools/merge-sweep.sh / tools/merge-sweep-cov.sh (targeting the orbit-api #403 race where reviewDecision survives an update-branch push while the re-triggered review check on the new head SHA is still running), plus prose guidance landed in .claude/skills/orchestrate/SKILL.md. This PR has already been through 7 review passes; five prior findings were fixed in later commits and are confirmed fixed in the current head (paginated + fail-closed workflow lookup, SHA-based rather than existence-based orphan detection, GraphQL-based orphan lookup that distinguishes confirmed-deleted from unknown). Two Medium findings from earlier passes were never addressed and are confirmed still present — re-raised below per the instruction to flag unresolved (not already-addressed) concerns.
Notably: at the time of this review, the PR's own reviewDecision field reads APPROVED while the review check for the current head SHA (this very review) is still IN_PROGRESS — a live instance of exactly the race this PR patches.
Findings
Critical: None.
High: None.
Medium:
tools/merge-sweep.sh:83-122— no fast-path SKIP whenreviewDecision != APPROVED, unlikemerge-sweep-cov.sh:113-117. A rejected PR just sleeps/re-polls for up to ~17 minutes before a generic timeout message instead of an immediateSKIP #$n review=$rev. Fix: add the same early check merge-sweep-cov.sh has, right after thefailed/DIRTYchecks.tools/merge-sweep.sh:109(andtools/merge-sweep-cov.sh:97insquash_merge) — the second, independentgh pr view --json headRefNamecall made right before merging can fail transiently, silently leavingbranchempty (swallowed by2>/dev/null, no exit-status check). The merge still proceeds, and the orphan-scan's[ -n "$branch" ] || continuedrops that entry without incrementingunverifiedor printing a WARN — the run can report exit 0 as if every merged head were verified clean, the exact fail-open shape this PR closes for the adjacentbranch_tip()lookup a few lines below. Fix: source the branch name from the existinggate/mstatepayload (already fetched every poll) instead of a second call; if a second call is kept, treat an empty result the same as a failedbranch_tiplookup (WARN + incrementunverified).
Both were raised in earlier review passes on this same PR and remain unaddressed in the current head; both are Medium (consistent with how this review history classified the closely analogous, already-fixed branch_tip() error-swallowing issue), so per the rubric's signal gate neither forces NEEDS WORK.
Subagents
All N/A — diff touches only .claude/ and tools/ (no apps/web, apps/mobile, orbit-api, packages/shared/src/types, or i18n JSON).
Validation
Phase 6 (/validate) skipped per instructions — CI runs Build/Unit Tests/SonarCloud separately as required checks. As of this review: Lint, Type Check, Build, Dash Ban, Copy Register, Suppressions Ratchet, Cross-Platform Parity, Contract Drift, CodeQL all SUCCESS; Unit Tests and SonarCloud Analysis were still IN_PROGRESS on the current head at review time (a prior SonarCloud run on an earlier SHA had passed cleanly, since superseded by the last merge-from-main commits). No orbit-api dimension to mark "not verifiable in CI" — the diff never touches that repo.
Deferred
Dimensions covering DESIGN.md, parity, i18n, contract drift, backend security, backend hard rules, FEATURES.md — N/A, no matching surface in the diff. All 6 changed files (.claude/pending-lessons.md, .claude/skills/orchestrate/SKILL.md, tools/README.md, tools/dash-baseline.json, tools/merge-sweep-cov.sh, tools/merge-sweep.sh) received a verdict.
What's good
- The core review-staleness fix is correct and applied to both sibling scripts: re-reads
reviewDecisionalongside thereviewcheck's settle state on the current head SHA every poll, gated before both the plain-merge and the Sonar coverage-only--adminoverride path. - Fail-closed workflow-lookup (
--paginate,review_required=1default, WARN on failure) correctly closes the earlier High finding (unpaginated + error-swallowed lookup). - Orphan-scan is SHA-based (not existence-based) via a GraphQL ref lookup that distinguishes a confirmed-deleted branch (empty oid) from an unverifiable one (non-zero exit → WARN + exit 3), correctly closing two earlier Medium findings.
--help, documented exit codes, andtools/README.mdcatalog rows matchtools/CONVENTIONS.md's contract; zero em/en dashes remain in either script, matching thedash-baseline.jsonshrink.
Recommendation
Approve as-is. The two Medium findings are real but narrow (wasted polling time; a rare transient-failure gap in a post-merge, best-effort audit), not safety-blocking — worth a fast follow-up rather than another review cycle on this already seven-times-reviewed PR.
|
The same hole, wider. subscribeToPushNotifications waits on the browser's permission prompt, which the person can sit on for as long as they like, and the shared auth cookie can change under them the whole time. Registering this browser's endpoint under whichever account signed in meanwhile would send that account's push notifications to a device they never armed. Both push writes now read the account when the intent forms, before the prompt, and carry it to the same server-side check the four inbox writes use. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The signal carried a sign in and nothing else, so a tab whose sibling signed the browser out kept the dead account's habits, goals and alerts on screen for whoever was at the keyboard next, for up to the full 60 seconds. That is the same window, unshrunk for half the transition. The payload now carries a null account for a sign out, and the receiving tab ends its session exactly the way the tab that pressed the button does. Both callers share endSessionLocally, so the two cannot drift. An unrecognised payload is still dropped rather than read as a sign out, because any tab on this origin can post on the channel and a stray message must not tear down a live session. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The signal was dropping. A channel opened per message and closed in the same turn delivered 0 of 200 messages here on 2026-09-18, where a channel left open delivered 200 of 200. Closing tears the port down before the runtime moves anything across it, so an announcement made that way reached nobody and the 60 second poll was still doing all the work. One channel now serves the tab for its life, opened on first use. A channel never receives its own posts, so this also removes the self delivery the two channel shape had, with no tab id to filter on. The listener teardown drops the listener and leaves the channel, because the tab keeps announcing its own transitions after the shell that was listening unmounts. The three test helpers that stood in for a second tab carried the same defect and now hold their channel open too. The signal suite ran eight times without a failure after the change, where it failed one run in four before it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Channel delivery runs on the event loop, not on a timer, so awaiting one macrotask proved nothing. Under the full suite's load the delivery landed after that turn and the first assertion in the signal file went red in one run of four, on working code. Every test now waits for the signal to actually arrive. A test that expects nothing posts a sentinel afterwards and waits for that instead: one channel delivers in order, so a sentinel that arrived with nothing before it proves the earlier message was dropped on purpose rather than merely late. The store and hook tests record on the same channel the store listens on, and both listeners run in one dispatch, so a signal the recorder has seen the store has seen too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Fix notification mutation failure feedback * Prevent stale notification delete errors * Clear delayed deletes at session boundaries * Fix root shell test type comparison * Guard notification rollback by session * Guard notification delete execution by session * Give the mobile test runtime the Expo globals The Expo runtime installs a global `expo` object through JSI and registers each native module on it before any JavaScript runs. Node gives Vitest no such runtime, so `expo-modules-core` read `globalThis.expo.EventEmitter` on an undefined global and every suite that reached an Expo module failed at import. `notification-inbox-ownership.test.tsx` was the first suite to reach SecureStore without a per-file mock, so it took CI red with zero of its own tests run. Install the global through `installExpoGlobalPolyfill`, the entry point Expo publishes for a test runtime, then register the `ExpoSecureStore` and `ExpoApplication` native modules from their real module definitions. Route the `expo` package root and `expo-sqlite` through shared doubles: the root entry requires a TypeScript file only Metro can rewrite, and SQLite is a native engine the test runtime does not reproduce. Two per-file `expo-secure-store` mocks existed only to get past the missing global and asserted nothing, so they go. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Gate every notification mutation by its owning session Rounds 3 and 4 bound only `useDeleteNotification` to the session that started it. The other three mutations kept writing without a session check: on both platforms `onError` restored the snapshot it took in `onMutate` and `onSettled` invalidated the list. TanStack Query runs those callbacks from the options snapshot the mutation captured, and `queryClient.clear()` empties the caches without cancelling a retryer, so a request account A started could reject after A signed out and B signed in. A's items then landed in B's cache and A's failure text was announced to B. Capture the session in `onMutate`, carry it in the mutation context, and gate the optimistic write, the rollback, the announcement and the invalidation for all four mutations on both platforms. Close the hole that defeated the gate itself. The web store read only `expiresAt`, so a tab could not see the shared cookie move to another account and kept the replaced account's generation. `/api/auth/session` now reports `userId`, decoded from the token the route already holds, and a changed account raises the generation, clears the pending notification deletes, rebinds step-up and drops the remembered user. A tab learning its account for the first time only records it, so a same-account reload is untouched. Both login paths clear the pending deletes as well. Move the duplicated guard to `packages/shared` as `createSessionScopedRunner` and give both stores one `getSessionEpoch(): number`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Empty the query cache when the cookie changes account `adoptSessionAccount` raised the session generation, cleared the pending notification deletes, rebound step-up and dropped the remembered user, but left the query cache untouched. Raising the generation stops the previous account's writes; nothing evicted its reads. A tab that switches account never navigates, so nothing else would either. With `staleTime` at 60 seconds and the notification poll at 5 minutes, tab one sat on account B while rendering account A's notification titles and bodies, habits, goals and profile. Clear the cache there, the way mobile already clears it at both account boundaries. Web logout needs no such call because it sets `globalThis.location.href` and the navigation discards the client outright. Also delete the `getSessionGeneration` export. Its only consumer moved to `getSessionEpoch`, which left two exported names for one value and no callers for one of them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Route every account change through one session path Web kept three writers for the account a tab holds and only one of them emptied the query cache, so a dead session followed by a login showed the previous account's notifications, habits, goals and profile. The three now route through one path that raises the generation, drops the pending deletes and empties the cache, and the last observed account outlives a teardown so the session after it can tell a return from a replacement. Every notification mutation now carries the session that started it into its request, so a clear-all that resumes after an account replacement no longer destroys the replacement account's notifications. A failed delayed delete now keeps its notice for a bounded life instead of stacking one more line onto every authenticated route for as long as the tab runs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Empty the Astra conversation when the account changes Route the chat store through the one path that changes which account a web tab holds, so a replacement account cannot read the previous account's conversation out of the module store while the shell stays mounted. Mobile already cleared it at both of its session boundaries, so this closes a parity break. Scope that path to a real account change: a teardown names no account, so it no longer counts as one, and a session that drops and returns as the same account keeps its cache instead of blanking the tab. Drop the ownership re-check after the cancel await in both notification hooks. Every caller wraps the work that follows in the session-scoped runner, which reads the current epoch at the point of each write, so the earlier check decided nothing and no test could redden on it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Empty the Astra composer when the account changes Zustand merges a partial set, so clearMessages left draft, draftRevision, draftHydrated and contextualSuggestion behind on both platforms. The composer rehydrates from storage whenever draftHydrated is false, so the stored draft goes with the state reset: localStorage on web, AsyncStorage on mobile. Also make startAccountScopedSession return void, since all three call sites discard the flag, and tidy two test helpers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Follow the session from the state a store cannot reach An account change that crosses a page load could not be recognised, so a sign out left the Astra draft in device storage for the next person, and the never unmounted composer kept the previous account's attempted send armed behind Retry. Every account transition now raises one session epoch and resets the Astra chat, a sign out included. The query cache alone stays gated on a real account change, because blanking a tab that the same account recovers costs that account its rows. React state that no store holds subscribes to the epoch, so the attempted send, the image and the text file go with the session. Mobile awaits the draft removal before the state reset, because clearing draftHydrated sends the composer straight back to storage and the Android storage module orders nothing; a rejected removal is reported rather than dropped. The mobile hydration effect now depends on draftHydrated, so a draft saves again after a sign in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Keep the Astra draft through a session that recovers The chat reset ran on every transition through startAccountScopedSession, so a rejected refresh that recovered as the same account threw away a half written Astra message. Round 4 settled the opposite for the query cache: a same account recovery must not blank the tab, and the composer is part of that tab. Gate the chat reset on a session STARTING under an account, and call it from logout. A teardown names no account, so a recoverable wobble now keeps the draft. A sign in still clears it, which is what closes the path a page load erases: lastObservedAccountId dies with the document, so a sign out followed by somebody else signing in reaches setAuth with no previous account to compare against. Mobile needs no change. It resets on every login and every teardown already, and it has no recoverable wobble: a transient refresh failure leaves the session mounted and tears nothing down. Two mobile assertions now hold that claim. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Forget the support draft and follow the account, not the session Both drafts a person types live under one key with no account in it. The Astra one is forgotten at every account transition; the support one was not, so the next person to sign in on the same browser or device read the previous person's subject and message back into the form and could send them under their own name. One module per platform now owns the support key, and the auth store forgets both drafts through a single path rather than at two call sites. The composer state no store can reach followed the session epoch, which rises on every credential change. A rejected refresh keeps the shell mounted behind the expiry banner, so a wobble that recovered as the same account revoked the pasted image and disarmed the retry while the person was still looking at them. An account generation now rises only where a draft is forgotten, and the composer hooks subscribe to that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Stop the failed delete clock while somebody is reading it Thomas settled that the failed delete toast keeps its ten seconds and pauses while a pointer rests on it or focus sits inside it, the same way the success toast already behaves. It gains no dismiss button and its one action stays Retry. The life ran on a bare timer inside a shared module that cannot see either, so the retry left the screen while the person was reaching for it. A neutral toast may now carry a life, and the notice hands it the ten seconds and the dismissal instead of running its own clock. The life is optional, so the other eight neutral callers are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Record the surfaces this branch moved The session identity, the account reset and the support draft storage sit in the import closure of every route and block that reaches the auth store, so 166 surface records no longer described the tree and the required drift check was red. None of the movement is visual: 716 closure sizes and one owned-file list change, with no surface added, removed or re-routed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Let the toast contract carry the life it was granted The type test still pinned neutral to no life at all, which is the rule Thomas replaced. It now asserts the opposite: a neutral toast may carry both the life and its end, and every other variant keeps its bans. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Take the previous account words off the screen, not only out of storage The support draft, the open notification and the revealed API key were all component state, so the account change that cleared their storage and their query cache left them rendered for whoever signs in next. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Retire one delete from pending before it is reported as failed The finally block emitted after the catch, so a synchronous throw published one snapshot holding the same id as both pending and failed. Cover the account claim the web session route reads with a token built from the payload the API issues. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Record the five closures the account reset moved Read the surface manifest back after regenerating it: the committed inventory matches the tree at 186 surfaces and 800 cells, and none of the movement is visual. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Cover the session counter where it lives The counter is shared code exercised only from the two apps, so its four functions never ran under the shared workspace coverage run and the function total tipped under its threshold at 95.98 against 96. The listener copy in advance gets the case that proves it: a callback that unsubscribes a sibling mid pass still lets that sibling run. Self unsubscription passes without the copy, because a Set skips only entries it has not visited yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Send the next account through onboarding on its own merits The retained onboarding guard froze whether the account had habits, and spent its one auto-complete, in state that outlives an account change because neither shell unmounts. A brand-new second account inherited the first account's answer and was auto-completed instead of onboarded. Both the snapshot and the spent attempt now carry the account generation they belong to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Close the cross-tab account window on notifications (#597) A tab learned that the shared auth cookie holds a different account only on its 60-second session poll. Inside that window a stale callback wrote the previous account's data into the cache, and, worse, an outbound notification write left under the next account's cookie, so DELETE /notifications emptied an inbox nobody asked about. Two mechanisms, because the two directions need different ones. Inbound: a BroadcastChannel signal announces the account a session starts under, and the receiving tab runs the same adoptSessionAccount path a detected change already runs. The 60-second poll stays as the fallback wherever the signal cannot arrive. Outbound: every notification write carries the account its intent was formed under, and serverAuthFetch refuses the request when the cookie it is about to send names somebody else. A client counter cannot decide this, because the counter says what the tab believes and the cookie says what the server acts on. Mobile gets no equivalent. One app has one runtime and no second tab, and each request reads the token from SecureStore and sends it as an explicit Bearer, so the credential is bound when the request is built. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore: regenerate the surface inventory for the cross-tab signal module (#597) The new web module joins every web surface's import closure, so the committed closure sizes no longer described the tree and the surface-manifest gate went red. The surface list itself is unchanged: 186 surfaces, 800 cells. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Gate the push subscription writes by account too (#597) The same hole, wider. subscribeToPushNotifications waits on the browser's permission prompt, which the person can sit on for as long as they like, and the shared auth cookie can change under them the whole time. Registering this browser's endpoint under whichever account signed in meanwhile would send that account's push notifications to a device they never armed. Both push writes now read the account when the intent forms, before the prompt, and carry it to the same server-side check the four inbox writes use. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Signal a sign out across tabs as well (#597) The signal carried a sign in and nothing else, so a tab whose sibling signed the browser out kept the dead account's habits, goals and alerts on screen for whoever was at the keyboard next, for up to the full 60 seconds. That is the same window, unshrunk for half the transition. The payload now carries a null account for a sign out, and the receiving tab ends its session exactly the way the tab that pressed the button does. Both callers share endSessionLocally, so the two cannot drift. An unrecognised payload is still dropped rather than read as a sign out, because any tab on this origin can post on the channel and a stray message must not tear down a live session. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Hold the account channel open for the life of the tab (#597) The signal was dropping. A channel opened per message and closed in the same turn delivered 0 of 200 messages here on 2026-09-18, where a channel left open delivered 200 of 200. Closing tears the port down before the runtime moves anything across it, so an announcement made that way reached nobody and the 60 second poll was still doing all the work. One channel now serves the tab for its life, opened on first use. A channel never receives its own posts, so this also removes the self delivery the two channel shape had, with no tab id to filter on. The listener teardown drops the listener and leaves the channel, because the tab keeps announcing its own transitions after the shell that was listening unmounts. The three test helpers that stood in for a second tab carried the same defect and now hold their channel open too. The signal suite ran eight times without a failure after the change, where it failed one run in four before it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Wait for the signal to arrive rather than for a timer (#597) Channel delivery runs on the event loop, not on a timer, so awaiting one macrotask proved nothing. Under the full suite's load the delivery landed after that turn and the first assertion in the signal file went red in one run of four, on working code. Every test now waits for the signal to actually arrive. A test that expects nothing posts a sentinel afterwards and waits for that instead: one channel delivers in order, so a sentinel that arrived with nothing before it proves the earlier message was dropped on purpose rather than merely late. The store and hook tests record on the same channel the store listens on, and both listeners run in one dispatch, so a signal the recorder has seen the store has seen too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore: regenerate the surface inventory at the merged head (#597) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Clears
.claude/pending-lessons.mdbefore Phase 7. Both queued entries were real failures that had never been acted on; each is routed to the tier its type calls for.Lesson 1 (checkable) becomes a gate, not prose
The race is real, and the current code does not handle it.
gh pr update-branchrewrites the head SHA and re-triggers thereviewcheck, butreviewDecisionis a PR-level field that survives the push, so it keeps reporting the pre-update APPROVED for as long as the re-review runs. Sincereviewis not a required check,mergeStateStatuscan reach CLEAN with Build/Tests/Sonar green while the Claude review is still executing. The sweeps re-readreviewDecisionevery poll, but every read in that window returns the stale APPROVED, so they merge. That is how a HIGH backend-contract finding reachedmainand deployed on orbit-api#403, with the fix landing on the orphaned head branch.Both sweeps now:
.github/workflows/claude-review.yml, and unless it positively does not, block every merge path (including the coverage-only admin override) until thereviewcheck on the current head SHA is terminal, then re-readreviewDecision.statusCheckRollupis scoped to the head commit, so a run from the pre-update SHA cannot satisfy the wait. A repo that positively has no such workflow (orbit-landing-page) never waits for a check that will not run;main: printed asORPHANED-HEAD, exit 1. A branch that merely survived--delete-branchwith an unchanged tip is benign and is not reported;--help, documented exit codes (0clean /1orphaned head /2bad usage) pertools/CONVENTIONS.md, and a timeout line that names the actual blocker instead of a generic "waiting for CLEAN".Why the precondition check itself fails closed
The first draft of this guard computed
review_requiredfrom an unpaginated, error-swallowedgh api repos/<repo>/actions/workflowscall and defaulted to OFF.gh apidoes not auto-paginate and that endpoint defaults toper_page=30, soclaude-review.ymlcould sit on page 2 in a repo with more than 30 workflows; separately,2>/dev/nullhid every auth, rate-limit and network failure. Either condition silently disabled the whole guard and restored the exact merge behaviour this PR exists to prevent, with no error line.It now uses
--paginateand inverts the default: required unless a successful lookup positively shows the workflow is absent, with a stderr warning when the lookup fails. That is the locked "Fail-closed the completion gate: a verifier error is not a clean pass" decision (2026-07-18, re-confirmed 2026-07-23 after a gate-tamper hook was found failing open) applied one level up. A guard whose own precondition check fails open is the same defect, just moved.merge-sweep.shcarried the same race in a stronger form (it merged the moment the state was decidable, without waiting for checks at all), so it is fixed in the same pass rather than left as a known-broken twin of the script being hardened.Proof the guard fires
Run against a stub
ghon PATH (stub + harness live in the scratchpad, never against real PRs). Case 1 feeds both the pre-change and post-change scripts the exact#403shape:mergeStateStatus=CLEAN,reviewDecision=APPROVED,reviewcheckIN_PROGRESSon the head SHA.Review findings addressed
--paginateplus an inverted default (required unless positively absent), with a stderr warning on lookup failure. Proven by case 4b.gh apifailures as "no orphan"Lesson 2 (judgment) becomes standing guidance
Landed in
.claude/skills/orchestrate/SKILL.md, under the existing Delegation discipline section, as "Waiting is foreground work, on both sides". That section is the one place in the repo carrying both a delegation contract template and a wait-on-CI loop (## 3. Babysit), so it is the only candidate home with a real consumer;.claude/rules/core.mdwas rejected because it is paid for on every turn of every session while this applies only when delegating.Both halves land, because the lesson records that the subagent-side warning alone had already failed twice:
Housekeeping
## Graduatedin.claude/pending-lessons.mdwith the date and where each landed; the queue is now empty.tools/README.mdcatalog rows updated for the changed behaviour and the new exit codes.tools/dash-baseline.jsonshrinks by 4 (the em dashes in the two scripts are gone).Verification